Skip to content

TC-735: persist KV blobs before the per-space sequence lock - #301

Open
samgbafa wants to merge 21 commits into
mainfrom
skgbafa/tc-735-prepersist-blobs
Open

samgbafa wants to merge 21 commits into
mainfrom
skgbafa/tc-735-prepersist-blobs

Conversation

@samgbafa

@samgbafa samgbafa commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Change

Content-addressed KV blobs persist before the per-space sequence lock. A short preparatory transaction checks authorization, hosted-space policy and write preconditions and durably registers one attempt per write. Ordinary uploads hold no pooled DB connection or transaction; the sequence guard is acquired after upload, then the short commit transaction rechecks the fence/attempt, assigns the sequence and commits the reference. kv/sync cursors remain commit-ordered. SpaceDatabase::transact and the KV post-upload transact(...) call (tinycloud-core/src/db.rs:1504-1533,2381-2482) still execute the epoch-head anti-join inside their sequence-held transactions; only the S3/FS upload leaves that hold, not the epoch query or index. TC-1274 owns epoch indexing and epoch_head changes. Protected legacy meeting writes retain a bounded pre-upload freeze barrier (TC-1260); a freeze ACK waits for DB commit or rollback, and post-ACK cleanup only removes unreferenced bytes.

An active (space,hash) fence and per-attempt rows protect overlapping uploads. The last unreferenced attempt moves its fence to deleting; a writer cannot dedup or upload over deleting bytes. DELETE is idempotent, confirmed physical absence refunds the process-local SpaceSizes exactly once, and an old delete cannot hit a later generation. Failed uploads/preconditions/DB commits clean up known unreferenced bytes. Crash leftovers become eligible after five minutes of DB time and a scheduled ≤60-second GC pass when no same-hash writer is active; the upload deadline is 240 seconds. Accepted live-overlap exception: a genuinely live same-space, same-hash upload protects the older crash stage and its charge; when the last overlapping attempt commits or aborts, that stage becomes due for the next scheduled pass (within 60 seconds). Repeated overlaps extend the delay. A backlog beyond the collector's four bounded pages can require later passes; this is a scheduling limit, not permission to delete or undercount. A committed blob is never deleted or refunded. Each bare/generation-scoped object represents at most its own physical bytes, while filesystem crash stages are separately charged physical files. Quota tracks physical rather than strictly committed bytes; multi-node shared accounting is a separate TC-1162 concern.

Filesystem memory staging publishes complete files atomically. A crash-leftover stage can be cleaned/refunded independently of a referenced final blob, retaining its recovery row through cleanup. An overlapping live same-hash writer protects its stage. Boot quota reconstruction tracks stage-file charges from the same listing as the total, then refunds a failed retry's reservation and the older stage independently on confirmed removal; known staged-upload failures refund immediately after unlink. Physical deletion and quota settlement continue even if the caller is cancelled. GC drains all expired attempts for one hash under the fence; each interval processes at most four 256-row pages. PostgreSQL page selection uses the existing retry index to scan in age order and a per-row LATERAL fence lock with SKIP LOCKED before LIMIT, then releases every lock before storage I/O. One held old hash cannot consume all page slots ahead of an unrelated orphan. A naive hash join instead scanned 100k rows and spilled to temp on PG16; the indexed LATERAL plan on the same fixture read only the selected 256 plus their fence rows, with no spill (~/scratch/tc-735/pg18-proof/orphan-held-fence-lateral-retry-order-pg16.log). Attempt created_at is immutable; nullable retry_after in this PR's unshipped _020000 migration schedules retries only. A failed read delays that claim without making it look live. A writer's commit or abort makes deferred claims due at the DB statement time without resetting them to never-retried priority; PostgreSQL commit wakeup is in the existing statement, not another round trip.

First-generation blobs use the old bare hash key (gen 0). Only a new upload after an orphan generation is deleted uses hash@N. An old 1.20.x binary cannot read an N>0 row after rollback. No known raw-blob production backup was found in Amos's records; provider-side config is unverified. Future bucket-level copy (including TC-950 S3→R2) should GC first or tolerate orphans.

Read boundary

The existing current_kv/kv_write lookup returns generation in the same query, and storage get/range derives the key. Gen-0 reads still use the bare key; GET remains 16 DB round trips without an extra allocation beyond the key string. read_audit.rs, invoke_read_only, authorization and audit are unchanged by this write fix. Separately reviewed #302 batches authorization snapshot reads and is required for this PR.

Main integration at 3ca9ceda693ef48272cf1d62f4595bf16839acf1 (Sam Gbafa)

  • 517297b merges Batch authorization graph snapshot in one SQL round trip (TC-735 dependency) #302 580abe5 (approved; alias-aware batched revocation, TC-1071) and main a735c4d. Startup still migrates before the S3 listing, so deleting fences are read before quota reconstruction. That migrate is now main's lock_timeout-bounded one (TC-1176). The database then opens through main's new_with_encryption, which finds the history current and runs the CID-alias backfill with the column key before serving requests.
  • 76ac385 merges main 3f85987. fix(TC-1354): size S3 block deletes with HeadObject #337 fixed S3 remove sizing with HeadObject. This branch already sizes and deletes through HeadObject with generation keys, per-object operation locks and confirmed-absence refunds, so its remove is kept and main's now-unused helpers are dropped. Main's TC-1354 MinIO test calls the generation-0 (bare-key) API.
  • c132567 merges main 1cbeb7d (feat(TC-1274): add epoch_order (space, parent, child) index migration #341). Under the second-merger rule, the unshipped migration is renamed m20261015_020000 → m20261017_020000_kv_orphan_candidates so it sorts after main's m20261017_000000_epoch_order_space_index. Main's cursor-index lock test replaces ours.
  • 9050d25 formats main's adapted S3 test (CI fmt failure at c132567).
  • 3ca9ced fixes the CI stack overflow (SIGABRT) in policy_v3::tests::policy_owner_may_be_a_session_the_space_owner_delegated_to at the default 2 MiB test stack. gdb at the overflow: the deepest path is routes::invoke → invoke_with_options_admitted → this branch's boxed commit block → db::transact → process_admitted → save. The transact debug frame (~188 KB) and the test's own frame (~936 KB) are the same on main; future state sizes did not grow. On main, transact runs under one ~129 KB invoke frame. Here it ran under the outer frame plus the commit block's 106,608-byte frame, because debug builds give every awaited temporary its own slot. The upload loop and the post-transact reads now live in persist_registered_blobs and invocation_side_effects, so the block frame is 78,208 bytes. The test's minimum passing stack drops from 2,056 KiB to 2,028 KiB (main 1cbeb7d: 2,032 KiB). Behavior is unchanged and the test stack is not raised. Main itself keeps only ~16 KiB of headroom on this test.
  • At 3ca9ced, local PG16/MySQL8/MinIO at the default 2 MiB test stack (no RUST_MIN_STACK; earlier local runs used 8 MiB, which hid the overflow): full serialized workspace suite 1,292 passed, 0 failed, 14 ignored; strict all-target Clippy and format pass; main's TC-1354 MinIO s3_block_store test passes; wire sign-in 69, KV PUT 42, GET 16. postgres_sql_statement_timeout_is_retryable_and_releases_the_lock passed 5/5 on clean main 1cbeb7d, 5/5 here and in both full suites. Its single failure at 517297b came during heavy host I/O and is not reproduced on either side. It stays unclassified, not called pre-existing.
  • Still required after the cut: the PG18.6 upgraded-1.20.6 joint proof of the migration rename on the actual release head. Exact-head CI is running.

Verification at 2800e9efeacf234217e27188672e02fedb976a32 (Sam Gbafa)

  • Full local serialized PostgreSQL 16.15 + MySQL 8.0.46 + MinIO workspace suite: 1,154 passed, 9 ignored (CI unset only for privileged runner-iptables cases), including 31 orphan tests, real MinIO storage/GC, postgres_same_space_commits_are_seq_ordered, conditional-put concurrency and the 69/42/16 round-trip budget. A two-pool PostgreSQL 16 RED→GREEN regression at logical 0/60/120 seconds holds 256 oldest X candidates behind a separate pool's fence: H retires while X is held, then X retires after release. It also passes on PostgreSQL 18.6. The overlap regression verifies a crash-leftover FS stage is reclaimed after a same-hash writer commits or aborts, without removing a committed final blob or double refund. Earlier filesystem cancellation, restart quota, freeze (20 repeated module passes) and cleanup tests remain green. Evidence: ~/scratch/tc-735/{held-fence-red-clean,held-fence-lateral-green-pg16,overlap-bound-fs-green,postgres-seq-final,postgres-conditional-final,full-suite-held-fence-lateral-green}.log. cargo fmt --all -- --check and strict all-target Clippy passed.
  • Wire budget: sign-in 69, KV PUT 42, GET 16 (PG16; earlier exact output artifact://1190; this follow-up changes only collector/wakeup SQL and tests, without adding a PUT round trip). Against today's 37-round-trip PUT, +5 is ~0.33s at ~65ms RTT when Batch authorization graph snapshot in one SQL round trip (TC-735 dependency) #302 is present; GET and sign-in each save six trips.
  • Isolated PostgreSQL 18.6 upgrade proof at parent 4cc4a4e (migration code unchanged in this head), upgraded 1.20.6 schema: db: page account session discovery #296 _000000 alone → TC-737 _010000 → TC-735 _020000, one migration per step; populated pre-current_kv BIGINT-generation backfill also passed. Synthetic integration 6698935 is not a TC-735: persist KV blobs before the per-space sequence lock #301 worktree edit. Fixture N>0 rows: kv_write=0, current_kv=0. Evidence: ~/scratch/tc-735/pg18-proof/{joint-upgrade-4cc4a4e,populated-old-kv-pg18-4cc4a4e}.log. On separate 100k-row fixtures, PG16 and PG18 use indexed attempt and fence lookups without temp spill for both never-retried and due-retry selections (orphan-held-fence-lateral-retry-order-{pg16,pg18}.log); PG18 selected 256 rows in 2.167/1.596 ms. The second merger must rebase/rename its migration to sort after everything on main and repeat PG18 proof on the actual release head.

Earlier PG18.6+MinIO same-space testing measured ~70 ops/s against ~93/180 targets, not below comparable main steady state. The epoch-head query remains inside the sequence-held transaction; TC-1274 owns its index/query work, not #301. This revision has not been retimed. A matched, warmed main+#302 versus candidate 8/32 same-space no-regression run happens after the release cut, in its own window; report numeric acceptance separately, not as a correctness pass.

Gates

#302 merges/ships before #301; otherwise PUT costs 51, not the accepted 42 round trips. Amos: #302 and #301 land after the current release cut. After the tag, each takes main, reruns CI and gets a short delta review, then rides the next release with its own window and smoke. Astra approved correctness at 2800e9e. Miller's reviewer passed 2800e9e and the bounded source delta to c132567; the stack fix delta (c132567..3ca9ced) needs a short review. Gates: exact-head CI, that delta review, the PG18 joint migration proof after the rename, and post-cut timing. TC-1176 holds node releases; no merge or deploy.

Post-cut follow-ups (from TC-1277 design; no code push until after the cut)

  1. has_references has no supporting index. It runs at every write commit and filters kv_write(space, value, generation) and current_kv(space, value, generation) WHERE NOT deleted. Add the indexes (a new migration, sorted after everything on main) and measure the commit query with EXPLAIN on a large fixture before and after.
  2. Generation naming must match TC-887's sealed names. This PR names generation N {space}/{b64(digest)}@N. feat(TC-887): seal KV blocks in the S3 store (S3c, stacked on #333) #336 names sealed objects v1/{space_tag}/{HMAC(NK, "obj"‖space‖multihash)} with no generation, so a sealed re-put after a delete would land on the deleted name. Whichever of TC-735: persist KV blobs before the per-space sequence lock #301 and feat(TC-887): seal KV blocks in the S3 store (S3c, stacked on #333) #336 lands second defines the sealed name for generation > 0 and adapts. feat(TC-887): seal KV blocks in the S3 store (S3c, stacked on #333) #336's remove must also delete every name of (space, hash, generation), legacy and sealed.
  3. The saturating size decrement is being ported to main separately; take main's version at the next merge.

@samgbafa

Copy link
Copy Markdown
Contributor Author

Correction on current head 5f34b4d: the PostgreSQL-backed workspace CI timeout is a TC-735 write-path freeze-ack regression, not a proven TC-1176 read-audit issue. legacy_freeze_ack_waits_for_actual_in_flight_kv_storage_and_commit (SQLite) timed out because a freeze acknowledged while its test upload was paused, the assertion panicked before releasing that upload, and teardown hung. Origin/main passed the single test and freeze module 20/20 each; a new PG16 delayed-upload RED test observed the early acknowledgement directly. A local, not-yet-pushed fix acquires the durable freeze row before upload, keeps sequence locking after upload, bounds freeze attempts to 30s, and rejects post-freeze writes before uploading. Added failed-upload refund/GC regression. Focused PG16 freeze module 13/13 and 20/20 repeated runs passed. Full PG-backed workspace suite and fresh CI/review are still required before this PR is ready; testing pauses for the TC-1149 20:35–21:05Z exclusive host window. No read_audit or nonmutating KV read changes, no merge/deploy. TC-735 root cause. The ~93/180 same-space throughput targets remain unmet (separate pre-existing epoch-head bottleneck TC-1218).

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735

Keep main's alias-aware revocation::effective_rows after the batched
snapshot (an extra round trip) so this merge alone preserves TC-1071;
the next commit folds it into the single statement. Production locking
now uses main's load_chain_lock_ids; load_closure_edges is a test oracle.
TC-1071 revokes every stored encoding sharing a canonical identity.
Add an alias-joined revocation branch to the single snapshot statement,
mirroring revocation::effective_rows, and drop the merge's extra query.
Regression: a revocation under another encoding is refused on SQLite,
PostgreSQL and MySQL. A seeded property test compares production
batched decisions with the sequential effective_rows snapshot over
random DAGs, alias groups, revoked and expired delegations.
Brings main a735c4d (TC-1071 CID-alias revocations, TC-1176 boot
lock_timeout, request telemetry) and #302 580abe5, whose batched auth
snapshot resolves alias revocations in its one statement.

Startup keeps the pre-S3-listing migration (now main's lock_timeout-
bounded migrate) so deleting fences are read before the quota listing,
then opens the database through main's new_with_encryption, which finds
migrations current and runs the CID-alias backfill with the column key
before serving requests.
TC-735's later kv_orphan_candidates migration made Migrator::down(1)
undo it instead of the cursor index. Identical to #341's fix.
#337 fixed S3 remove sizing with HeadObject; this branch already sizes
and deletes through HeadObject with generation keys, operation locks and
confirmed-absence refunds, so keep its remove and drop main's now-unused
helpers. Main's TC-1354 MinIO test uses generation 0 (the bare key).
Rename the unshipped kv_orphan_candidates migration from
m20261015_020000 to m20261017_020000 so it sorts after main's
m20261017_000000_epoch_order_space_index, per the second-merger rule.
Take main's cursor-index lock test (migrated_through), dropping ours.
CI aborted policy_owner_may_be_a_session_the_space_owner_delegated_to
on the 2 MiB test stack. gdb at the overflow: the deepest path is
invoke -> boxed commit block -> transact -> process_admitted -> save.
transact's debug frame (~188 KB) and the test's own frame (~936 KB)
are the same as main. This branch adds the boxed commit block's frame
(106,608 bytes) under the outer invoke frame. In debug builds every
awaited temporary in that block has its own stack slot.

Move the upload loop and the post-transact reads into their own
methods, so their temporaries are on the stack only while they run.
The block's frame is now 78,208 bytes. The test's minimum passing
stack falls from 2,056 KiB to 2,028 KiB (main: 2,032 KiB). The test
stack is unchanged.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant